feat: add composite type support for field resolvers #1341
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
…or-graphql-operation
…or-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
…or-graphql-operation
…or-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…eration' of github.com:wundergraph/graphql-go-tools into ludwig/eng-8297-add-interface-support-for-field-resolvers
… ludwig/eng-8297-add-interface-support-for-field-resolvers
WalkthroughAdds support for composite types (interfaces and unions) in the gRPC datasource execution planning system, including enhanced field resolver handling, AST utilities, new RPC operations for test infrastructure, stack data structure, visitor pattern rework using ancestor stacks, and comprehensive tests for nested/composite resolver scenarios. Changes
Sequence DiagramsequenceDiagram
participant Query
participant PlanVisitor
participant ResolverStack as fieldResolverAncestors
participant ExecPlan as Execution Plan
Query->>PlanVisitor: EnterSelectionSet
alt Is Field Resolver
PlanVisitor->>ResolverStack: enterFieldResolver()
ResolverStack->>ResolverStack: push(resolvedField)
alt Is Composite Type
PlanVisitor->>ExecPlan: enterResolverCompositeSelectionSet()
ExecPlan->>ExecPlan: getMemberTypes()
ExecPlan->>ExecPlan: buildCompositeField()
ExecPlan->>ExecPlan: buildFieldMessage()
else Regular Field
PlanVisitor->>ExecPlan: resolveRequiredFields()
ExecPlan->>ExecPlan: buildRequiredField()
end
else Regular Field
PlanVisitor->>ExecPlan: Process normally
end
PlanVisitor->>ResolverStack: LeaveField()
ResolverStack->>ResolverStack: pop()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Noroth
requested review from
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.gov2/pkg/grpctest/productv1/product_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
v2/pkg/ast/ast_selection.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(6 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(10 hunks)v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/util.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/util_test.go(1 hunks)v2/pkg/grpctest/mapping/mapping.go(9 hunks)v2/pkg/grpctest/mockservice.go(1 hunks)v2/pkg/grpctest/product.proto(9 hunks)v2/pkg/grpctest/testdata/products.graphqls(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
📚 Learning: 2025-08-25T14:44:39.420Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1283
File: v2/pkg/grpctest/product.proto:14-15
Timestamp: 2025-08-25T14:44:39.420Z
Learning: In the gRPC test mapping system in v2/pkg/grpctest/mapping/mapping.go, entity lookup RPCs (like LookupWarehouseById) belong in EntityRPCs mapping but should NOT be added to QueryRPCs. Entity RPCs and Query RPCs serve different purposes in the mapping architecture.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/product.proto
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/grpctest/product.proto
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
🧬 Code graph analysis (11)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (5)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (11)
RPCExecutionPlan(57-62)RPCCall(78-94)CallKindEntity(71-71)RPCMessage(98-112)RPCField(183-212)CallKindResolve(73-73)OneOfType(29-29)OneOfTypeInterface(36-36)RPCFieldSelectionSet(149-149)OneOfTypeUnion(38-38)NewPlanner(333-351)v2/pkg/engine/datasource/grpc_datasource/configuration.go (1)
GRPCMapping(26-46)v2/pkg/engine/datasource/grpc_datasource/compiler.go (6)
DataTypeMessage(38-38)Message(182-186)DataTypeString(29-29)DataTypeBool(36-36)DataTypeInt32(30-30)DataTypeDouble(35-35)v2/pkg/grpctest/schema.go (1)
MustGraphQLSchema(75-87)v2/pkg/astparser/parser.go (1)
ParseGraphqlDocumentString(37-45)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
RPCExecutionPlan(57-62)OneOfTypeNone(34-34)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/ast/path.go (1)
Path(34-34)
v2/pkg/ast/ast_selection.go (1)
v2/pkg/ast/ast.go (1)
Document(10-56)
v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go (2)
v2/pkg/ast/ast.go (1)
Document(10-56)v2/pkg/engine/plan/required_fields_visitor.go (1)
RequiredFieldsFragment(33-35)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
v2/pkg/ast/ast_selection.go (3)
SelectionSet(21-25)SelectionKindField(16-16)SelectionKindInlineFragment(18-18)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (5)
v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/ast/path.go (1)
Path(34-34)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
OneOfTypeNone(34-34)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinition(17-26)v2/pkg/operationreport/externalerror.go (1)
ExternalError(34-42)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (9)
RPCExecutionPlan(57-62)RPCCall(78-94)RPCMessage(98-112)RPCField(183-212)CallKindResolve(73-73)OneOfType(29-29)OneOfTypeInterface(36-36)RPCFieldSelectionSet(149-149)OneOfTypeUnion(38-38)v2/pkg/engine/datasource/grpc_datasource/compiler.go (7)
DataTypeMessage(38-38)Message(182-186)DataTypeString(29-29)DataTypeDouble(35-35)DataTypeEnum(37-37)DataTypeBool(36-36)DataTypeInt32(30-30)
v2/pkg/grpctest/mockservice.go (1)
v2/pkg/grpctest/productv1/product.pb.go (113)
ResolveProductMascotRecommendationRequest(5945-5953)ResolveProductMascotRecommendationRequest(5966-5966)ResolveProductMascotRecommendationRequest(5981-5983)ResolveProductMascotRecommendationResponse(6043-6048)ResolveProductMascotRecommendationResponse(6061-6061)ResolveProductMascotRecommendationResponse(6076-6078)ResolveProductMascotRecommendationResult(5999-6004)ResolveProductMascotRecommendationResult(6017-6017)ResolveProductMascotRecommendationResult(6032-6034)Animal(9291-9300)Animal(9313-9313)Animal(9328-9330)Animal_Cat(9361-9363)Animal_Cat(9369-9369)Cat(11337-11345)Cat(11358-11358)Cat(11373-11375)Animal_Dog(9365-9367)Animal_Dog(9371-9371)Dog(11405-11413)Dog(11426-11426)Dog(11441-11443)ResolveProductProductDetailsRequest(6437-6445)ResolveProductProductDetailsRequest(6458-6458)ResolveProductProductDetailsRequest(6473-6475)ResolveProductProductDetailsResponse(6535-6540)ResolveProductProductDetailsResponse(6553-6553)ResolveProductProductDetailsResponse(6568-6570)ResolveProductProductDetailsResult(6491-6496)ResolveProductProductDetailsResult(6509-6509)ResolveProductProductDetailsResult(6524-6526)ActionResult(10311-10320)ActionResult(10333-10333)ActionResult(10348-10350)ActionResult_ActionError(10385-10387)ActionResult_ActionError(10391-10391)ActionError(11525-11531)ActionError(11544-11544)ActionError(11559-11561)ActionResult_ActionSuccess(10381-10383)ActionResult_ActionSuccess(10389-10389)ActionSuccess(11473-11479)ActionSuccess(11492-11492)ActionSuccess(11507-11509)ProductDetails(10765-10773)ProductDetails(10786-10786)ProductDetails(10801-10803)ResolveProductStockStatusRequest(6191-6199)ResolveProductStockStatusRequest(6212-6212)ResolveProductStockStatusRequest(6227-6229)ResolveProductStockStatusResponse(6289-6294)ResolveProductStockStatusResponse(6307-6307)ResolveProductStockStatusResponse(6322-6324)ResolveProductStockStatusResult(6245-6250)ResolveProductStockStatusResult(6263-6263)ResolveProductStockStatusResult(6278-6280)QueryTestContainerRequest(4090-4095)QueryTestContainerRequest(4108-4108)QueryTestContainerRequest(4123-4125)QueryTestContainerResponse(4135-4140)QueryTestContainerResponse(4153-4153)QueryTestContainerResponse(4168-4170)TestContainer(10155-10162)TestContainer(10175-10175)TestContainer(10190-10192)QueryTestContainersRequest(4180-4184)QueryTestContainersRequest(4197-4197)QueryTestContainersRequest(4212-4214)QueryTestContainersResponse(4217-4222)QueryTestContainersResponse(4235-4235)QueryTestContainersResponse(4250-4252)ResolveTestContainerDetailsRequest(8333-8341)ResolveTestContainerDetailsRequest(8354-8354)ResolveTestContainerDetailsRequest(8369-8371)ResolveTestContainerDetailsResponse(8431-8436)ResolveTestContainerDetailsResponse(8449-8449)ResolveTestContainerDetailsResponse(8464-8466)ResolveTestContainerDetailsResult(8387-8392)ResolveTestContainerDetailsResult(8405-8405)ResolveTestContainerDetailsResult(8420-8422)TestDetails(11577-11585)TestDetails(11598-11598)TestDetails(11613-11615)ResolveCategoryMetricsNormalizedScoreRequest(8095-8103)ResolveCategoryMetricsNormalizedScoreRequest(8116-8116)ResolveCategoryMetricsNormalizedScoreRequest(8131-8133)ResolveCategoryMetricsNormalizedScoreResponse(8193-8198)ResolveCategoryMetricsNormalizedScoreResponse(8211-8211)ResolveCategoryMetricsNormalizedScoreResponse(8226-8228)ResolveCategoryMetricsNormalizedScoreResult(8149-8154)ResolveCategoryMetricsNormalizedScoreResult(8167-8167)ResolveCategoryMetricsNormalizedScoreResult(8182-8184)ResolveCategoryMascotRequest(7381-7389)ResolveCategoryMascotRequest(7402-7402)ResolveCategoryMascotRequest(7417-7419)ResolveCategoryMascotResponse(7479-7484)ResolveCategoryMascotResponse(7497-7497)ResolveCategoryMascotResponse(7512-7514)ResolveCategoryMascotResult(7435-7440)ResolveCategoryMascotResult(7453-7453)ResolveCategoryMascotResult(7468-7470)CategoryKind_CATEGORY_KIND_OTHER(32-32)CategoryKind_CATEGORY_KIND_BOOK(29-29)CategoryKind_CATEGORY_KIND_ELECTRONICS(30-30)ResolveCategoryCategoryStatusRequest(7619-7627)ResolveCategoryCategoryStatusRequest(7640-7640)ResolveCategoryCategoryStatusRequest(7655-7657)ResolveCategoryCategoryStatusResponse(7717-7722)ResolveCategoryCategoryStatusResponse(7735-7735)ResolveCategoryCategoryStatusResponse(7750-7752)ResolveCategoryCategoryStatusResult(7673-7678)ResolveCategoryCategoryStatusResult(7691-7691)ResolveCategoryCategoryStatusResult(7706-7708)
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (1)
v2/pkg/engine/datasource/grpc_datasource/configuration.go (2)
FieldMapData(81-84)FieldArgumentMap(87-87)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
Message(182-186)NewProtoCompiler(317-360)v2/pkg/grpctest/schema.go (2)
MustGraphQLSchema(75-87)MustProtoSchema(98-102)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
NewDataSource(64-83)DataSourceConfig(53-61)
v2/pkg/grpctest/mapping/mapping.go (1)
v2/pkg/engine/datasource/grpc_datasource/configuration.go (2)
FieldMapData(81-84)FieldArgumentMap(87-87)
🔇 Additional comments (1)
v2/pkg/ast/ast_selection.go (1)
215-224: Nice addition to the selection helpersThe helper cleanly complements
SelectionSetFieldSelectionsand aligns with existing selection utilities without altering behaviour elsewhere.
Comment on lines
+1122
to
+1127
| name: "Should create an execution plan for a query with interface type", | ||
| query: "query CategoriesWithNestedResolvers($includeValue: Boolean!) { categories { mascot(includeVolume: $includeVolume) { ... on Cat { name } ... on Dog { name } } } }", | ||
| expectedPlan: &RPCExecutionPlan{ | ||
| Calls: []RPCCall{ | ||
| { | ||
| ServiceName: "Products", |
There was a problem hiding this comment.
Fix undefined variable in composite resolver query
The query declares $includeValue but the field argument references $includeVolume, so validation will fail before the planner runs. Please align the variable name with the argument.
Apply this diff:
- query: "query CategoriesWithNestedResolvers($includeValue: Boolean!) { categories { mascot(includeVolume: $includeVolume) { ... on Cat { name } ... on Dog { name } } } }",
+ query: "query CategoriesWithNestedResolvers($includeVolume: Boolean!) { categories { mascot(includeVolume: $includeVolume) { ... on Cat { name } ... on Dog { name } } } }",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: "Should create an execution plan for a query with interface type", | |
| query: "query CategoriesWithNestedResolvers($includeValue: Boolean!) { categories { mascot(includeVolume: $includeVolume) { ... on Cat { name } ... on Dog { name } } } }", | |
| expectedPlan: &RPCExecutionPlan{ | |
| Calls: []RPCCall{ | |
| { | |
| ServiceName: "Products", | |
| name: "Should create an execution plan for a query with interface type", | |
| query: "query CategoriesWithNestedResolvers($includeVolume: Boolean!) { categories { mascot(includeVolume: $includeVolume) { ... on Cat { name } ... on Dog { name } } } }", | |
| expectedPlan: &RPCExecutionPlan{ | |
| Calls: []RPCCall{ | |
| { | |
| ServiceName: "Products", |
🤖 Prompt for AI Agents
In
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
around lines 1122 to 1127, the GraphQL query declares the variable $includeValue
but the field argument uses $includeVolume which causes validation to fail; fix
it by making the variable name and the argument match (either rename the
declared variable to $includeVolume or change the field argument to use
$includeValue) so the query compiles and the test planner runs.
Comment on lines
+1418
to
+1526
| Name: "ResolveTestContainerDetailsRequest", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "context", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveTestContainerDetailsContext", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "id", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "id", | ||
| ResolvePath: buildPath("test_containers.id"), | ||
| }, | ||
| { | ||
| Name: "name", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "name", | ||
| ResolvePath: buildPath("test_containers.name"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "field_args", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveTestContainerDetailsArgs", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "include_extended", | ||
| ProtoTypeName: DataTypeBool, | ||
| JSONPath: "includeExtended", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "ResolveTestContainerDetailsResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "result", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "result", | ||
| Repeated: true, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveTestContainerDetailsResult", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "details", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "details", | ||
| Optional: true, | ||
| Message: &RPCMessage{ | ||
| Name: "TestDetails", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "id", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "id", | ||
| }, | ||
| { | ||
| Name: "summary", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "summary", | ||
| }, | ||
| { | ||
| Name: "pet", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "pet", | ||
| Message: &RPCMessage{ | ||
| Name: "Animal", | ||
| OneOfType: OneOfTypeInterface, | ||
| MemberTypes: []string{"Cat", "Dog"}, | ||
| FieldSelectionSet: RPCFieldSelectionSet{ | ||
| "Cat": { | ||
| { | ||
| Name: "name", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "name", | ||
| }, | ||
| { | ||
| Name: "meow_volume", | ||
| ProtoTypeName: DataTypeInt32, | ||
| JSONPath: "meowVolume", | ||
| }, | ||
| }, | ||
| "Dog": { | ||
| { | ||
| Name: "name", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "name", | ||
| }, | ||
| { | ||
| Name: "bark_volume", | ||
| ProtoTypeName: DataTypeInt32, | ||
| JSONPath: "barkVolume", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Use GraphQL response casing in resolver buildPath lookups
Every buildPath("test_containers...") here should follow the GraphQL field casing (testContainers) like the rest of the suite. Keeping the proto-style snake_case causes the expected plan to differ from the actual plan, so these tests will always fail. Update all of the resolver context paths accordingly.
Apply this diff (abbreviated to show the required replacements):
- ResolvePath: buildPath("test_containers.id"),
+ ResolvePath: buildPath("testContainers.id"),
- ResolvePath: buildPath("test_containers.name"),
+ ResolvePath: buildPath("testContainers.name"),
…
- ResolvePath: buildPath("test_containers.id"),
+ ResolvePath: buildPath("testContainers.id"),
- ResolvePath: buildPath("test_containers.name"),
+ ResolvePath: buildPath("testContainers.name"),
…
- ResolvePath: buildPath("test_containers.id"),
+ ResolvePath: buildPath("testContainers.id"),
- ResolvePath: buildPath("test_containers.name"),
+ ResolvePath: buildPath("testContainers.name"),Make the same replacement for every occurrence within this test block so the expectations match the planner output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Name: "ResolveTestContainerDetailsRequest", | |
| Fields: []RPCField{ | |
| { | |
| Name: "context", | |
| ProtoTypeName: DataTypeMessage, | |
| Repeated: true, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsContext", | |
| Fields: []RPCField{ | |
| { | |
| Name: "id", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "id", | |
| ResolvePath: buildPath("test_containers.id"), | |
| }, | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| ResolvePath: buildPath("test_containers.name"), | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| Name: "field_args", | |
| ProtoTypeName: DataTypeMessage, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsArgs", | |
| Fields: []RPCField{ | |
| { | |
| Name: "include_extended", | |
| ProtoTypeName: DataTypeBool, | |
| JSONPath: "includeExtended", | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Response: RPCMessage{ | |
| Name: "ResolveTestContainerDetailsResponse", | |
| Fields: []RPCField{ | |
| { | |
| Name: "result", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "result", | |
| Repeated: true, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsResult", | |
| Fields: []RPCField{ | |
| { | |
| Name: "details", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "details", | |
| Optional: true, | |
| Message: &RPCMessage{ | |
| Name: "TestDetails", | |
| Fields: []RPCField{ | |
| { | |
| Name: "id", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "id", | |
| }, | |
| { | |
| Name: "summary", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "summary", | |
| }, | |
| { | |
| Name: "pet", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "pet", | |
| Message: &RPCMessage{ | |
| Name: "Animal", | |
| OneOfType: OneOfTypeInterface, | |
| MemberTypes: []string{"Cat", "Dog"}, | |
| FieldSelectionSet: RPCFieldSelectionSet{ | |
| "Cat": { | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| }, | |
| { | |
| Name: "meow_volume", | |
| ProtoTypeName: DataTypeInt32, | |
| JSONPath: "meowVolume", | |
| }, | |
| }, | |
| "Dog": { | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| }, | |
| { | |
| Name: "bark_volume", | |
| ProtoTypeName: DataTypeInt32, | |
| JSONPath: "barkVolume", | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Name: "ResolveTestContainerDetailsRequest", | |
| Fields: []RPCField{ | |
| { | |
| Name: "context", | |
| ProtoTypeName: DataTypeMessage, | |
| Repeated: true, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsContext", | |
| Fields: []RPCField{ | |
| { | |
| Name: "id", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "id", | |
| ResolvePath: buildPath("testContainers.id"), | |
| }, | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| ResolvePath: buildPath("testContainers.name"), | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| Name: "field_args", | |
| ProtoTypeName: DataTypeMessage, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsArgs", | |
| Fields: []RPCField{ | |
| { | |
| Name: "include_extended", | |
| ProtoTypeName: DataTypeBool, | |
| JSONPath: "includeExtended", | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Response: RPCMessage{ | |
| Name: "ResolveTestContainerDetailsResponse", | |
| Fields: []RPCField{ | |
| { | |
| Name: "result", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "result", | |
| Repeated: true, | |
| Message: &RPCMessage{ | |
| Name: "ResolveTestContainerDetailsResult", | |
| Fields: []RPCField{ | |
| { | |
| Name: "details", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "details", | |
| Optional: true, | |
| Message: &RPCMessage{ | |
| Name: "TestDetails", | |
| Fields: []RPCField{ | |
| { | |
| Name: "id", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "id", | |
| }, | |
| { | |
| Name: "summary", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "summary", | |
| }, | |
| { | |
| Name: "pet", | |
| ProtoTypeName: DataTypeMessage, | |
| JSONPath: "pet", | |
| Message: &RPCMessage{ | |
| Name: "Animal", | |
| OneOfType: OneOfTypeInterface, | |
| MemberTypes: []string{"Cat", "Dog"}, | |
| FieldSelectionSet: RPCFieldSelectionSet{ | |
| "Cat": { | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| }, | |
| { | |
| Name: "meow_volume", | |
| ProtoTypeName: DataTypeInt32, | |
| JSONPath: "meowVolume", | |
| }, | |
| }, | |
| "Dog": { | |
| { | |
| Name: "name", | |
| ProtoTypeName: DataTypeString, | |
| JSONPath: "name", | |
| }, | |
| { | |
| Name: "bark_volume", | |
| ProtoTypeName: DataTypeInt32, | |
| JSONPath: "barkVolume", | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
lines 1418-1526: tests use buildPath("test_containers...") (snake_case) but the
planner produces GraphQL-style camelCase; update every occurrence in this block
to use buildPath("testContainers...") (e.g., test_containers.id ->
testContainers.id, test_containers.name -> testContainers.name) so the resolver
context paths match the expected planner output.
Comment on lines
+406
to
+429
| // Field arguments for non root types will be handled as resolver calls. | ||
| // We need to make sure to handle a hierarchy of arguments in order to perform parallel calls in order to retrieve the data. | ||
| fieldArgs := r.operation.FieldArguments(ref) | ||
| // We don't want to add fields from the selection set to the actual call | ||
| resolvedField := resolvedField{ | ||
| callerRef: r.parentCallID, | ||
| parentTypeNode: r.walker.EnclosingTypeDefinition, | ||
| fieldRef: ref, | ||
| responsePath: r.walker.Path[1:].WithoutInlineFragmentNames().WithFieldNameItem(r.operation.FieldAliasOrNameBytes(ref)), | ||
| fieldDefinitionTypeRef: r.definition.FieldDefinitionType(fieldDefRef), | ||
| } | ||
|
|
||
| if err := r.planCtx.setResolvedField(r.walker, fieldDefRef, fieldArgs, r.fieldPath, &resolvedField); err != nil { | ||
| r.walker.StopWithInternalErr(err) | ||
| return | ||
| } | ||
|
|
||
| r.resolvedFields = append(r.resolvedFields, resolvedField) | ||
| r.fieldResolverAncestors.push(len(r.resolvedFields) - 1) | ||
| r.fieldPath = r.fieldPath.WithFieldNameItem(r.operation.FieldNameBytes(ref)) | ||
|
|
||
| // In case of nested fields with arguments, we need to increment the related call ID. | ||
| r.parentCallID++ | ||
| } |
There was a problem hiding this comment.
Same parentCallID leak under federation
The federation visitor mirrors the resolver handling from the non-federated planner: enterFieldResolver increments parentCallID, but LeaveField only decrements it when the field has arguments. A resolver without arguments leaves the counter bumped, so the very next resolver in the same scope binds to the wrong caller. The federation path needs the same fix—store/restore parentCallID (e.g., via a stack) so every resolver, regardless of arguments, reports the correct parent call.
Comment on lines
+443
to
+466
| // Field arguments for non root types will be handled as resolver calls. | ||
| // We need to make sure to handle a hierarchy of arguments in order to perform parallel calls in order to retrieve the data. | ||
| fieldArgs := r.operation.FieldArguments(ref) | ||
| // We don't want to add fields from the selection set to the actual call | ||
| resolvedField := resolvedField{ | ||
| callerRef: r.parentCallID, | ||
| parentTypeNode: r.walker.EnclosingTypeDefinition, | ||
| fieldRef: ref, | ||
| responsePath: r.walker.Path[1:].WithoutInlineFragmentNames().WithFieldNameItem(r.operation.FieldAliasOrNameBytes(ref)), | ||
| fieldDefinitionTypeRef: r.definition.FieldDefinitionType(fieldDefRef), | ||
| } | ||
|
|
||
| if err := r.planCtx.setResolvedField(r.walker, fieldDefRef, fieldArgs, r.fieldPath, &resolvedField); err != nil { | ||
| r.walker.StopWithInternalErr(err) | ||
| return | ||
| } | ||
|
|
||
| r.resolvedFields = append(r.resolvedFields, resolvedField) | ||
| r.fieldResolverAncestors.push(len(r.resolvedFields) - 1) | ||
| r.fieldPath = r.fieldPath.WithFieldNameItem(r.operation.FieldNameBytes(ref)) | ||
|
|
||
| // In case of nested fields with arguments, we need to increment the related call ID. | ||
| r.parentCallID++ | ||
| } |
There was a problem hiding this comment.
Restore parentCallID after resolver fields
enterFieldResolver increments parentCallID, but the only place that ever decrements it is inside LeaveField when FieldHasArguments(ref) is true. Any resolver that has no arguments – which is fully supported in the schema – will leave parentCallID permanently incremented. The next sibling resolver will then record the wrong callerRef, so its follow-up RPC is wired to the previous resolver’s response instead of the original parent call. Please snapshot the current value before the increment (or keep a dedicated stack) and restore it unconditionally when leaving the resolver field so siblings always inherit the correct caller.
🤖 Prompt for AI Agents
In v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go around
lines 443 to 466, enterFieldResolver increments r.parentCallID but it is only
decremented conditionally in LeaveField when FieldHasArguments(ref) is true,
causing resolvers without arguments to permanently shift callerRef for
subsequent siblings; snapshot the current parentCallID before incrementing (or
push it on a dedicated stack), then restore the saved value unconditionally when
leaving the resolver field so every sibling gets the correct callerRef (i.e.,
save oldParent, increment for nested resolver work, and always restore oldParent
on exit).
Comment on lines
+1131
to
+1145
| fieldRefs := r.operation.SelectionSetFieldRefs(fragmentSelection.selectionSetRef) | ||
| result := make([]RPCField, 0, len(fieldRefs)) | ||
|
|
||
| for _, fieldRef := range fieldRefs { | ||
| fieldDef, found := r.fieldDefinitionForType(r.operation.FieldNameString(fieldRef), fragmentSelection.typeName) | ||
| if !found { | ||
| return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", r.operation.FieldNameString(fieldRef)) | ||
| } | ||
|
|
||
| field, err := r.buildField(inlineFragmentNode, fieldDef, r.operation.FieldNameString(fieldRef), "") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result = append(result, field) | ||
| } |
There was a problem hiding this comment.
Inline fragment fields drop their nested messages
When an inline fragment field has a subselection, we never recurse into it. buildCompositeField returns the RPCField directly, so field.Message stays nil, and the generated request omits all nested structure for that selection. Any resolver returning an object through an inline fragment will therefore lose the required child fields at runtime.
Please mirror the recursion we already perform in buildRequiredField:
@@
- for _, fieldRef := range fieldRefs {
- fieldDef, found := r.fieldDefinitionForType(r.operation.FieldNameString(fieldRef), fragmentSelection.typeName)
- if !found {
- return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", r.operation.FieldNameString(fieldRef))
- }
-
- field, err := r.buildField(inlineFragmentNode, fieldDef, r.operation.FieldNameString(fieldRef), "")
+ for _, fieldRef := range fieldRefs {
+ fieldName := r.operation.FieldNameString(fieldRef)
+ fieldDef, found := r.fieldDefinitionForType(fieldName, fragmentSelection.typeName)
+ if !found {
+ return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", fieldName)
+ }
+
+ field, err := r.buildField(inlineFragmentNode, fieldDef, fieldName, "")
if err != nil {
return nil, err
}
+ if field.ProtoTypeName == DataTypeMessage && r.operation.FieldHasSelections(fieldRef) {
+ fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDef))
+ if !found {
+ return nil, fmt.Errorf("unable to build composite field: unable to resolve field type node for field %s", fieldName)
+ }
+ message, err := r.buildFieldMessage(fieldTypeNode, fieldRef)
+ if err != nil {
+ return nil, err
+ }
+ field.Message = message
+ }
result = append(result, field)
}This preserves the nested message structure for inline fragment fields and keeps the resolver output complete.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fieldRefs := r.operation.SelectionSetFieldRefs(fragmentSelection.selectionSetRef) | |
| result := make([]RPCField, 0, len(fieldRefs)) | |
| for _, fieldRef := range fieldRefs { | |
| fieldDef, found := r.fieldDefinitionForType(r.operation.FieldNameString(fieldRef), fragmentSelection.typeName) | |
| if !found { | |
| return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", r.operation.FieldNameString(fieldRef)) | |
| } | |
| field, err := r.buildField(inlineFragmentNode, fieldDef, r.operation.FieldNameString(fieldRef), "") | |
| if err != nil { | |
| return nil, err | |
| } | |
| result = append(result, field) | |
| } | |
| fieldRefs := r.operation.SelectionSetFieldRefs(fragmentSelection.selectionSetRef) | |
| result := make([]RPCField, 0, len(fieldRefs)) | |
| for _, fieldRef := range fieldRefs { | |
| fieldName := r.operation.FieldNameString(fieldRef) | |
| fieldDef, found := r.fieldDefinitionForType(fieldName, fragmentSelection.typeName) | |
| if !found { | |
| return nil, fmt.Errorf("unable to build composite field: field definition not found for field %s", fieldName) | |
| } | |
| field, err := r.buildField(inlineFragmentNode, fieldDef, fieldName, "") | |
| if err != nil { | |
| return nil, err | |
| } | |
| if field.ProtoTypeName == DataTypeMessage && r.operation.FieldHasSelections(fieldRef) { | |
| fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDef)) | |
| if !found { | |
| return nil, fmt.Errorf("unable to build composite field: unable to resolve field type node for field %s", fieldName) | |
| } | |
| message, err := r.buildFieldMessage(fieldTypeNode, fieldRef) | |
| if err != nil { | |
| return nil, err | |
| } | |
| field.Message = message | |
| } | |
| result = append(result, field) | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
testContainer,testContainers,mascotRecommendation,stockStatus,productDetails,normalizedScore, and related fields across Product, Category, and CategoryMetrics types.ProductDetails,TestContainer, andTestDetailsto support richer data relationships.Tests
Checklist